Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added additional estimates from GR website #247

Closed
wants to merge 14 commits into from

Conversation

malcolm-dsider
Copy link
Collaborator

@malcolm-dsider malcolm-dsider commented Jun 26, 2024

Added estimate for property tax, royalties, and govt royalties

Also other miscellaneous updates


Relevant to #232

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build is failing - https://github.com/NREL/GEOPHIRES-X/actions/runs/9685364851/job/26725280505?pr=247

Unit tests will also fail once syntax error is addressed since calculations are changed but examples are not updated

DefaultValue=1.0,
Min=0.0,
Max=10.0,
UnitType=Units.PERCENT,
PreferredUnits=PercentUnit.TENTH,
CurrentUnits=PercentUnit.TENTH,
ErrMessage="assume default cylindrical reservoir radius of effect reduction factor (0.1)",
ErrMessage="assume default cyclindrical reservoir radius of effect reduction factor (0.1)",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo "cyclindrical"

@@ -92,13 +96,14 @@ def __init__(self, model: Model):
)
self.RadiusOfEffectFactor = self.ParameterDict[self.RadiusOfEffectFactor.Name] = floatParameter(
"Cylindrical Reservoir Radius of Effect Factor",
value=1.0,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove - only DefaultValue should be specified, not value (which is redundant with DefaultValue in this case anyway)

@@ -80,6 +83,7 @@ def __init__(self, model: Model):
)
self.RadiusOfEffect = self.ParameterDict[self.RadiusOfEffect.Name] = floatParameter(
"Cylindrical Reservoir Radius of Effect",
value=30.0,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove - only DefaultValue should be specified, not value (which is redundant with DefaultValue in this case anyway)

@@ -68,6 +70,7 @@ def __init__(self, model: Model):
)
self.Length = self.ParameterDict[self.Length.Name] = floatParameter(
"Cylindrical Reservoir Length",
value=4.0,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove - only DefaultValue should be specified, not value (which is redundant with DefaultValue in this case anyway)

@@ -56,6 +57,7 @@ def __init__(self, model: Model):

self.OutputDepth = self.ParameterDict[self.OutputDepth.Name] = floatParameter(
"Cylindrical Reservoir Output Depth",
value=self.InputDepth.value,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove - only DefaultValue should be specified, not value (which is redundant with DefaultValue in this case anyway)

Comment on lines +1646 to +1648
f.write(f' Estimated Property tax that will be paid: {model.economics.property_tax_created.value:10.2f }' + model.economics.property_tax_created.PreferredUnits.value + NL)
f.write(f' Estimated total royalties that will be paid: {model.economics.total_royalties_created.value:10.2f }' + model.economics.total_royalties_created.PreferredUnits.value + NL)
f.write(f' Estimated government royalties to be paid: {model.economics.gov_royalties_created.value:10.2f }' + model.economics.gov_royalties_created.PreferredUnits.value+ NL)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there better/more precise phrasing that can eliminate the long and clunky "to be paid"/"that will be paid" suffixes here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will work on the phrasing

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But do you like the additional calculations based on the information used to generate the estimate of "jobs created?"

Comment on lines +1851 to +1853
UnitType=Units.JOBS,
CurrentUnits=JobsUnit.JOBS,
PreferredUnits=JobsUnit.JOBS
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the value in defining a specific unit for this - counts are unitless and represented as Units.NONE i.e.

UnitType=Units.NONE,

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yah, I was struggling with the code during the class, so I added this just to get it run. I will set it back to what is right - but this code does work... just doesn't make alot of sense.

LCOH = self.LCOH.value * 2.931 # $/MMBTU
LCOH = LCOH * 2.931 # $/MMBTU
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change doesn't seem possible without impacting example outputs, but no example output changes are in the PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a one line bug I found some weeks ago and committed but forgot to do a PR, so it got folded into this PR. This bug showed itself in only a couple of cases, but the fix is correct. There may be some output changes, but maybe not.

Comment on lines -247 to +258
pressure=self.hydrostatic_pressure()
pressure=model.reserv.lithostatic_pressure()
)
self.rhowater.value = density_water_kg_per_m3(
model.wellbores.Tinj.value * 0.5 + (self.Trock.value * 0.9 + model.wellbores.Tinj.value * 0.1) * 0.5,
pressure=self.hydrostatic_pressure()
pressure=model.reserv.lithostatic_pressure()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We just changed these from lithostatic to hydrostatic last month (#217 / c77f418), is this change back intentional? If so, what's the rationale?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had merge conflicts during the class which I had to resolve quickly. We should go back to hydrostatic.

Comment on lines +364 to +367
class RoyaltyPerEnergyUnit(str,Enum):
"""Royalty per energy Units"""
ROYALTYPERMW = "MUSD/MW"
ROYALTYPERKW = "MUSD/kW"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is redundant and/or inconsistent with EnergyCostUnit

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, but I will check.

@malcolm-dsider
Copy link
Collaborator Author

malcolm-dsider commented Jun 26, 2024 via email

@softwareengineerprogrammer
Copy link
Collaborator

Relevant tracking issue: #169

@softwareengineerprogrammer
Copy link
Collaborator

Closing per sync discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants